This change is 

I wonder if we might been better off re-thinking the API which took a *kv.Txn to make them indirect through an object to hold the transaction or what not.

I agree -- passing the ie along the txn through layers is torturing :)
I tried modified structs that might match what you mentioned:

// SQLTranslator is the concrete implementation of spanconfig.SQLTranslator.
type SQLTranslator struct {
	ptsProvider protectedts.Provider
	codec       keys.SQLCodec
	knobs       *spanconfig.TestingKnobs
	txnBundle txnBundle
}

// txnBundle is created to emphasize that the SQL translator is correspond to
// a certain txn, and all fields here are a whole. It essentially keeps the
// semantics of “translate at a snapshot in time”. This means that this
// txnBundle should be written only in `NewTranslator`.
type txnBundle struct {
	txn      *kv.Txn
	descsCol *descs.Collection
	ie sqlutil.InternalExecutor
}

Do you think this is what you would prefer?

That commit itself is fine, but I don't really understand the exported getters. I think the next step would then be to give the protected timestamp interfaces a similar treatment such that when interacting with protected timestamps you don't think about which txn you're using either.

Is this roughly close to what you were thinking? ZhouXing19@4de105e#diff-25c1841c99ae0297b18c357c13b913e454b2cafb70cc7fa557326dc11a1ff892R49

Not quite what I had in mind. I was not expecting there to be any setters or any exported logic that is aware of whether anything was set.

I guess it is like what I had in mind in that you removed the Txn argument from GetMetadata

Imagine if the Txn argument was removed from all of the methods in that interface.

I wonder where we should limit the scope of this refactor / updating internalExecutor usages. The redesign being proposed here seems like something that would be better suited for the owners of the protectedts code. Part of the reason I was advocating for an earlier announcement of the new internal executor interfaces was so that the work to use the new interfaces and discover opportunities to improve interfaces would come to the teams that own the various parts of the code that use the internal executor.

It does not seem tenable to have Jane (or more generally, SQL engineers) make somewhat substantial refactors in any area of the code just because it so happens to touch the internal executor. I recall earlier when Jane pushed back against having to make the updates for all the internal executor usages because it would be a giant undertaking, the response was that it was a lot of changes, but all mechanical, and mechanical refactors are good. It seems to me that we're going further away from just mechanical refactors.

Concretely: I would like Jane's project to not continue to balloon in scope too much. I feel that a way to achieve that is to keep the refactoring here mechanical, and leave any deeper questions of how the protectedts package should plumb different objects up to the code owners. Thoughts?

I feel that a way to achieve that is to keep the refactoring here mechanical, and leave any deeper questions of how the protectedts package should plumb different objects up to the code owners. Thoughts?

I buy the sentiment. It was where I started when I first saw the PR. I was going to write that you have my stamp. I have a harder time imagining that unnamed codeowners are going to come around and do anything unless there are names and deadlines.

What really got me to rethink here was that it feels to me like the mechanical refactors could be made smaller with some initial refactors elsewhere. These nominally mechanical changes do introduce new txn loops right-and-left and then require capturing things in closure scope that dilutes the intention of the code. It seems pretty clear how to rework these interfaces to avoid the fundamental flaws and if we don't do it, this codebase is reaching the point where we'll never do them.

I think that if we want to ease the burden, what we should do is create two tasks:

1. Move the optional txn from the sprawling set of methods on the *jobs.Job object to an intermediate structure
2. Remove the optional txn from the sprawing set of methods on the protectedts.Storage interface out to an intermediate structure

Then say that progress is blocked on that. I'm interested in helping with that. It's hard for me to find the time to do that in the next week.

I reference fatalism above because it's a feeling that I get when thinking about these various choices. I want to at least have some courage to ask us to understand more deeply why this refactor hurts and what other future refactor we might need to do will hurt just as much, or more because we're not attacking the root cause of the inflexibility. What's going on here is to couple, philosophically, sql transactional concepts with other parts of the sql subsystem like via the internal executor.

Having written the above statement, perhaps what we ought to do is to make a new struct which can hold these two objects. That might make this change more mechanical. I still don't feel like it's better than decomposing the dependencies from the arguments, but it'd be something.

I think that if we want to ease the burden, what we should do is create two tasks:

• Move the optional txn from the sprawling set of methods on the *jobs.Job object to an intermediate structure
• Remove the optional txn from the sprawing set of methods on the protectedts.Storage interface out to an intermediate structure

Maybe we can just replace the txn in those methods' argument with sqlutil.InternalExecutor?

For the 2nd part, I think we can remove the txn from the arguments of those methods, as the txn is only used for internal executor query functions. What about we have a ie.GetTxn() as a (temporary) replacement in these query functions?

I.e. can we have

func (p *storage) UpdateTimestamp(
	ctx context.Context,  id uuid.UUID, timestamp hlc.Timestamp, ie sqlutil.InternalExecutor,
) error {
	row, err := ie.QueryRowEx(ctx, "protectedts-update", ie.GetTxn(),
		sessiondata.InternalExecutorOverride{User: username.NodeUserName()},
		updateTimestampQuery, id.GetBytesMut(), timestamp.WithSynthetic(false).AsOfSystemTime())
	if err != nil {
		return errors.Wrapf(err, "failed to update record %v", id)
	}
	if len(row) == 0 {
		return protectedts.ErrNotExists
	}
	return nil
}

For the 1st part I think it's the same, but a bit more implicit. I looked at all usages of Job.Update() and most of txn arguments are useless, or they got passed to methods in protectedts.Storage. So similarly, we can just have

func (j *Job) Update(ctx context.Context, ie sqlutil.InternalExecutor, updateFn UpdateFn) error {
	const useReadLock = false
	return j.update(ctx, ie, useReadLock, updateFn)
}

If we want to do this, it seems still pretty mechanical to me and I don't think intermediate structures are needed anymore.
But still, this means that we're still refactoring stuff through the usages of methods/functions. It just makes it look cleaner , but the number of changes, I assume, is pretty much the same.

Maybe we can just replace the txn in those methods' argument with sqlutil.InternalExecutor?

Continuing on this but on a higher level, do you think it makes sense that txn should always be encompassed by an internal executor, rather than being passed in parallel with an internal executor to methods?

I wonder if we might been better off re-thinking the API which took a *kv.Txn to make them indirect through an object to hold the transaction or what not.

Can p.ex be injected directly with the correct InternalExecutor across method boundaries as opposed to threading it?

Given that txn might not be needed for Job.Update() and methods of protectedts.Storage, can we do this:

// Job manages logging the progress of long-running system processes, like
// backups and restores, to the system.jobs table.
type Job struct {
	// TODO(benesch): avoid giving Job a reference to Registry. This will likely
	// require inverting control: rather than having the worker call Created,
	// Started, etc., have Registry call a setupFn and a workFn as appropriate.
	registry *Registry
	ex *sqlutil.InternalExecutor

	id        jobspb.JobID
	createdBy *CreatedByInfo
	session   sqlliveness.Session
	mu        struct {
		syncutil.Mutex
		payload  jobspb.Payload
		progress jobspb.Progress
		status   Status
		runStats *RunStats
	}
}

func (j *Job) WithTxn(txnBoundInternalExecutor sqlutil.InternalExecutor, f func() error) error {
	j.ex = txnBoundInternalExecutor
	defer func() { j.ex = nil }()
	return f()
}

// storage interacts with the durable state of the protectedts subsystem.
type storage struct {
	settings *cluster.Settings

	knobs *protectedts.TestingKnobs
	ie sqlutil.InternalExecutor
}

func (s *storage) WithTxn(txnBoundInternalExecutor sqlutil.InternalExecutor, f func()) {
	s.ie = txnBoundInternalExecutor
	defer func() { s.ie = nil }()
	f()
}

So that the internal executor hangs off Job or storage only in a certain closure, to avoid getting misused under a txn that it's not bound to.

Per an offline convo with @ajwerner we've decided to pause on this work. We need refactoring of the internal executor interface and related functions in sqlutil. Essentially, there needs to be a better binding of *kv.Txn and sqlutil.InternalExecutor, and fewer functions with closures.
I'll go back to this PR and the remaining migration after the refactoring is done.

I've been hacking on it in the background. It'll take a little more focused time, but I think the end state that's coming together is starting to feel good. Thanks for kicking off the conversation with this.

Closing this as it has been done by #93218.